chore: support json transaction files for debugging #483
Conversation
anshumankomawar
force-pushed
the
dev/anshumankomawar
branch
from
0429185 to
346b197
Compare
| METAFILE_EXT = { | ||
| "json": ".json", | ||
| "msgpack": ".mpk", | ||
| }[METAFILE_FORMAT] |
There was a problem hiding this comment.
Nice - that's a clever way to refactor this constant. :)
deltacat/storage/model/metafile.py
Outdated
| :return: Deserialized object from the metadata file. | ||
| """ | ||
| if format not in {"json", "msgpack"}: |
There was a problem hiding this comment.
Nit: Can we add a new SUPPORTED_METAFILE_FORMATS constant to save this set in?
deltacat/storage/model/metafile.py
Outdated
| :return: Deserialized object from the metadata file. | ||
| """ | ||
| if format not in {"json", "msgpack"}: | ||
| raise ValueError( | ||
| f"Unsupported format '{format}'. Must be 'json' or 'msgpack'." |
There was a problem hiding this comment.
Nit: If we create the above constant, then we can just change this to something like:
| f"Unsupported format '{format}'. Must be 'json' or 'msgpack'." | |
| f"Unsupported format '{format}'. Supported formats include: {SUPPORTED_METAFILE_FORMATS}." |
deltacat/storage/model/metafile.py
Outdated
| """ | ||
| if format not in {"json", "msgpack"}: |
There was a problem hiding this comment.
Same nit here about using a constant to save the supported formats.
deltacat/constants.py
Outdated
| @@ -37,6 +38,15 @@ | |||
| False, | |||
| ) | |||
|
|
|||
| # CLI Args | |||
| METAFILE_FORMAT_KEY = "metafile-format" | |||
There was a problem hiding this comment.
Prefer to use standard environment variable casing here:
| METAFILE_FORMAT_KEY = "metafile-format" | |
| METAFILE_FORMAT_KEY = "METAFILE_FORMAT" |
deltacat/utils/common.py
Outdated
|
|
||
|
|
||
| def env_bool(key: str, default: bool) -> bool: | ||
| cli_value = get_cli_arg(key) |
There was a problem hiding this comment.
Hmm... since you can also set environment variables within the context of a single bash command, can we just let deltacat devs do that and not overload the environment variables here to either look for CLI args or environment variables?
(i.e., $ METAFILE_FORMAT=json python ./my-script.py)
We can then create a more formal/unified deltacat CLI experience within the context of a dedicated CLI application, similar to dev/deploy/aws/scripts/runner.py.
Signed-off-by: Patrick Ames <[email protected]>
* chore: support json transaction files for debugging * chore: support optional file format for metafiles * refactor: remove uncessary print * pr: remove cli arg dependency use os env instead * fix: revert changes to common.py --------- Signed-off-by: Patrick Ames <[email protected]> Co-authored-by: Patrick Ames <[email protected]>
Summary
This PR introduces support for specifying the
METAFILE_FORMATusing an environment variable.Usage
The added
metafile-formatcan be used likeTesting
make testRegression Risk
If this is a bugfix, assess the risk of regression caused by this fix and steps taken to mitigate it.
Checklist
Unit tests covering the changes have been added
E2E testing has been performed